-
Notifications
You must be signed in to change notification settings - Fork 64
starknet_os_flow_tests: localize expected storage updates in test_os_logic #9485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
starknet_os_flow_tests: localize expected storage updates in test_os_logic #9485
Conversation
862a7c8 to
3f5741e
Compare
54669b5 to
331d175
Compare
3f5741e to
c37d283
Compare
331d175 to
eccbbad
Compare
c37d283 to
f82250a
Compare
eccbbad to
da2f80a
Compare
6ba2a44 to
3de0895
Compare
55c8825 to
93ca899
Compare
3de0895 to
ac793d6
Compare
93ca899 to
b5d5d74
Compare
5f4fc35 to
65aee29
Compare
b5d5d74 to
e619f3e
Compare
65aee29 to
cd53bd6
Compare
e619f3e to
b252323
Compare
cd53bd6 to
bce8534
Compare
b252323 to
e874fe5
Compare
e874fe5 to
661ee19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@meship-starkware reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @Yoni-Starkware)
crates/starknet_os_flow_tests/src/tests.rs line 424 at r4 (raw file):
}) .or_insert_with(|| HashMap::from([(key, value)])); };
Why, as an inline function, is it only relevant for this test?
Code quote:
let mut update_expected_storage = |address: ContractAddress, key: Felt, value: Felt| {
let key = StarknetStorageKey(StorageKey(key.try_into().unwrap()));
let value = StarknetStorageValue(value);
expected_storage_updates
.entry(address)
.and_modify(|map| {
map.insert(key, value);
})
.or_insert_with(|| HashMap::from([(key, value)]));
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @meship-starkware, and @Yoni-Starkware)
crates/starknet_os_flow_tests/src/tests.rs line 424 at r4 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Why, as an inline function, is it only relevant for this test?
yes, IIRC I didn't have use for this in another test, though I may be wrong... if I am please block me, I'll extract into a shared util.
also, inlining this means I don't have to inject expected_storage_updates as a parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! all files reviewed, all discussions resolved
crates/starknet_os_flow_tests/src/tests.rs line 424 at r4 (raw file):
Previously, dorimedini-starkware wrote…
yes, IIRC I didn't have use for this in another test, though I may be wrong... if I am please block me, I'll extract into a shared util.
also, inlining this means I don't have to injectexpected_storage_updatesas a parameter
I took a look at the test, and it seems like this is the only one with a large storage update, except for meta tx, but this function will not help there. So I think we can keep it as is

No description provided.